-
Notifications
You must be signed in to change notification settings - Fork 3.8k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
opt: check that generator functions are not used in CASE or COALESCE #105582
Conversation
Hold off on this one for a bit; I think I added to many restrictions for what expressions can exist inside a COALESCE or CASE. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @DrewKimball)
-- commits
line 12 at r1:
Can you mention IF
expressions too? We build those as CaseExprs in optbuilder, so I don't think you'll need to change any code - though might be good to add some IF
test cases like SELECT IF(false, generate_series(1, 3), 1)
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is RFAL.
Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @mgartner)
Previously, mgartner (Marcus Gartner) wrote…
Can you mention
IF
expressions too? We build those as CaseExprs in optbuilder, so I don't think you'll need to change any code - though might be good to add someIF
test cases likeSELECT IF(false, generate_series(1, 3), 1)
.
Done.
4a59fa7
to
75d32a0
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewed 1 of 3 files at r1, 17 of 17 files at r2, all commit messages.
Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @DrewKimball, @renatolabs, and @srosenberg)
pkg/sql/logictest/testdata/logic_test/srfs
line 1383 at r2 (raw file):
# IFNULL does not allow generator functions. statement error pq: set-returning functions are not allowed in COALESCE
Is COALESCE
a typo, or is this message due to a normalization of IFNULL to a COALESCE?
pkg/sql/opt/optbuilder/testdata/aggregate
line 3636 at r2 (raw file):
SELECT array_agg(generate_series(1, 2)) ---- error (0A000): array_agg(): set-returning functions are not allowed in aggregate
Is this change intentional? I don't see anything in the release note mentioning aggregate functions.
This patch adds checks during type-checking to ensure that generator functions are not used in the arguments of `CASE`, `IF`, `COALESCE`, or `IFNULL` expressions. This mirrors postgres behavior. This patch also corrects the error message that is returned in these cases to say "set-returning" instead of "generator". Fixes cockroachdb#97119 Fixes cockroachdb#94890 Release note (bug fix): CASE, IF, COALESCE, and IFNULL expressions now return an error when passed a generator function as an argument. This mirrors postgres behavior.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @mgartner, @renatolabs, and @srosenberg)
pkg/sql/logictest/testdata/logic_test/srfs
line 1383 at r2 (raw file):
Previously, mgartner (Marcus Gartner) wrote…
Is
COALESCE
a typo, or is this message due to a normalization of IFNULL to a COALESCE?
This is because we parse IFNULL
directly as a COALESCE
, so there's never even a tree
expression for IFNULL
. Expanded the comment.
pkg/sql/opt/optbuilder/testdata/aggregate
line 3636 at r2 (raw file):
Previously, mgartner (Marcus Gartner) wrote…
Is this change intentional? I don't see anything in the release note mentioning aggregate functions.
This change is correct, we weren't catching this case previously because replaceSRF
was called before we checked if generator functions were allowed. Now srf.TypeCheck
also checks if the context allows generators, so we throw the error.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewed 1 of 1 files at r3, all commit messages.
Reviewable status: complete! 1 of 0 LGTMs obtained (waiting on @renatolabs and @srosenberg)
pkg/sql/logictest/testdata/logic_test/srfs
line 1383 at r2 (raw file):
Previously, DrewKimball (Drew Kimball) wrote…
This is because we parse
IFNULL
directly as aCOALESCE
, so there's never even atree
expression forIFNULL
. Expanded the comment.
👍
pkg/sql/opt/optbuilder/testdata/aggregate
line 3636 at r2 (raw file):
Previously, DrewKimball (Drew Kimball) wrote…
This change is correct, we weren't catching this case previously because
replaceSRF
was called before we checked if generator functions were allowed. Nowsrf.TypeCheck
also checks if the context allows generators, so we throw the error.
👍
TFTR! bors r+ |
Build succeeded: |
A change in cockroachdb#105582 caused the regions/replicas query to return incorrect, unexpected results causing the endpoint to never resolve properly, which in turn, caused an infinite loading state. This change fixes the regions/replicas query for the database details API. Release note (ui change): fix a broken query for the database details page that was causing an infinite loading state.
107893: cluster-ui: fix replicas and regions query for the database details API r=THardy98 a=THardy98 Epic: None A change in #105582 caused the regions/replicas query to return incorrect, unexpected results (instead of reducing the returned rows from the CTE to a single row, the cross join caused the number of rows to square, resulting in x^2 rows) causing the endpoint to never resolve properly, which in turn, caused an infinite loading state. This change fixes the regions/replicas query for the database details API. Release note (ui change): fix a broken query for the database details page that was causing an infinite loading state. Co-authored-by: Thomas Hardy <thardy@cockroachlabs.com>
This commit fixes a bug introduced in cockroachdb#105582 that caused SemaRejectFlags to be restored during semantic analysis, preventing the analysis from detecting some forms of invalid expressions. Fixes cockroachdb#108166 There is no release note because the related bug does not exist in any releases. Release note: None
This commit fixes a bug introduced in cockroachdb#105582 that caused SemaRejectFlags to be restored during semantic analysis, preventing the analysis from detecting some forms of invalid expressions. Fixes cockroachdb#108166 There is no release note because the related bug does not exist in any releases. Release note: None
108188: sql: refactor semantic analysis and fix some bugs r=mgartner a=mgartner #### sql/sem/tree: simplify SemaCtx reject flag checks Release note: None #### sql/sem/tree: split derived SemaContext properties from contextual info Properties derived about expressions during semantic analysis are communicated to callers via ScalarProperties. Prior to this commit, this type was also used to provide contextual information while traversing sub-expressions during semantic analysis. For example, it would indicate whether the current expression is a descendent of a window function expression. These two types of information, derived and contextual, are fundamentally different. Derived properties bubble up from the bottom of the tree to the top, while context propagates downward into sub-expressions. This difference made it difficult to maintaining them correctly in a single type and difficult to reason about. This commit introduces the ScalarScene type which is used for providing internal contextual information during semantic analysis. Release note: None #### sql/sem/tree: do not Restore SemaRejectFlags during semantic analysis This commit fixes a bug introduced in #105582 that caused SemaRejectFlags to be restored during semantic analysis, preventing the analysis from detecting some forms of invalid expressions. Fixes #108166 There is no release note because the related bug does not exist in any releases. Release note: None #### sql: do not allow subqueries to be cast to enums in views and UDFs This commit is a follow-up to #106868 after additional reproductions of the original bug were found. For now, we disallow any CAST expressions that contain a subquery in the input and the target type is an ENUM. I've created #108184 to track this limitation. Fixes #107654 There is no release note because the release note from #106868 should be sufficient. Release note: None #### sql/randgen: fix typo in comment Release note: None Co-authored-by: Marcus Gartner <marcus@cockroachlabs.com>
A change in cockroachdb#105582 caused the regions/replicas query to return incorrect, unexpected results causing the endpoint to never resolve properly, which in turn, caused an infinite loading state. This change fixes the regions/replicas query for the database details API. Release note (ui change): fix a broken query for the database details page that was causing an infinite loading state.
opt: check that generator functions are not used in CASE or COALESCE
This patch adds checks during type-checking to ensure that generator functions
are not used in the arguments of
CASE
,IF
,COALESCE
, orIFNULL
expressions. This mirrors postgres behavior. This patch also corrects the error
message that is returned in these cases to say "set-returning" instead of
"generator".
Fixes #97119
Fixes #94890
Release note (bug fix): CASE, IF, COALESCE, and IFNULL expressions now return
an error when passed a generator function as an argument. This mirrors postgres
behavior.